feat(rivetkit): add ConnectionMap readonly Map wrapper for actor connections#5021
Conversation
|
🚅 Deployed to the rivet-pr-5021 environment in rivet-frontend
|
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
3d96914 to
ccf09c6
Compare
ef2c9d3 to
d69dfaa
Compare
Code Review:
|
| Issue | Severity |
|---|---|
Duplicate use rivet_error::ActorSpecifier import (compile error) |
Blocker |
as MapIterator<T> unsound cast |
High |
N+1 actorConns calls per chained operation |
Medium |
New adapter object identity per get/values call |
Medium |
ConnectionMap/NativeConnAdapter missing from public exports |
Medium |
callNativeSync now exported |
Low |
The concept is sound — a live ReadonlyMap view over connections is cleaner than an eagerly-snapshotted Map. The duplicate import must be fixed before this can land, and the MapIterator cast and adapter identity issues need addressing.
|
Review of feat(rivetkit): add ConnectionMap readonly Map wrapper for actor connections. This PR replaces the eager Map built in the conns getter with a lazy NativeConnectionMap that implements ReadonlyMap. Issues found: (1) Performance - every Map method calls actorConns() independently through NAPI, so composite patterns like has+get pay 2x NAPI trips and 2x O(n) scans instead of 1 trip + O(1) lookup. (2) Snapshot inconsistency - map.size and map.entries() can disagree if connections change between calls. (3) Unnecessary exports of callNativeSync and NativeConnAdapter with no current consumers. (4) Return type leaks the concrete NativeConnectionMap class; it should be ReadonlyMap. (5) Unsound MapIterator casts - Array iterators are not MapIterators. Suggestion: add a private snapshot() helper that builds a real Map once per call - fixes O(n) regression, eliminates snapshot inconsistency, removes unsound casts. Also narrow the conns return type to ReadonlyMap, remove unnecessary exports, and add tests. Minor: the ReadonlyMap constraint is a good improvement over the prior mutable Map return type. |
d69dfaa to
5014be8
Compare
ccf09c6 to
f268b35
Compare
5014be8 to
6eedbc8
Compare
6eedbc8 to
8e1538c
Compare

Description
Please include a summary of the changes and the related issue. Please also include relevant motivation and context.
Type of change
How Has This Been Tested?
Please describe the tests that you ran to verify your changes.
Checklist: